test(uds): wait on the request lock instead of sleeping - #186
Conversation
UdsClientImpl and UdsFunctionalClient take _requestLock through a new AcquireRequestLockAsync helper. It raises the internal RequestLockContended event when the lock is already held, and UdsClientImpl also raises RequestLockAcquired once it is taken. Tests use them in place of wall-clock sleeps timed to land while another call holds the lock (#171). Neither event has a subscriber in production. The zero-timeout probe takes the caller's token, so a call cancelled before it starts throws rather than taking a free lock, as the plain WaitAsync it replaces did. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
Converts the lock and queue rows of the UDS audit (#171, audit item 7) from wall-clock sleeps to the request-lock observables: - UdsClientTests: Dispose_During_InFlight_Request_Does_Not_Race_RequestLock, Dispose_Cancels_Suppress_TesterPresent_Blocked_On_RequestLock and SecurityAccess_Holds_Lock_Across_Seed_And_Key. The last one now needs a keep-alive tick to have actually queued behind SecurityAccess; before, it counted 30 ms periods and could pass with no tick at all. - UdsExpiredDeadlineTests: N_Dispose_Leaves_The_Lock_To_A_Holder_That_Outlasts_The_Wait. - UdsFunctionalClientTests: A_Call_Queued_Behind_Another_Does_Not_Send_After_Dispose. - UdsTransferTests: DownloadAsync_Holds_Exclusive_Lock_Against_Concurrent_TesterPresent. Both TesterPresent calls must be seen queued on the lock while the download holds it, replacing the 2 ms race width. The test asserts that this happened. Adds tests for a pre-cancelled call on a free lock and for a contended lock with no subscriber. An_Invalid_Collection_Window_Transmits_Nothing keeps its 50 ms window, with a comment explaining why: it is a negative check on another bus, and a regression's frame would reach that bus asynchronously. Refs #171 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
PR SummaryLow Risk Overview Production (internal only): Tests: Lock/queue scenarios (SecurityAccess vs keep-alive, download vs concurrent TesterPresent, dispose races, functional client queue behind an open window) synchronize on those events instead of millisecond sleeps. New coverage includes uncontended contention with no subscriber and no lock taken when cancelled upfront. Reviewed by Cursor Bugbot for commit 5119a36. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
What does this change?
This is the second step of #171. It covers the UDS lock and queue rows from item 7 of the delay/sleep audit.
Several UDS tests slept for a fixed time and hoped another call had reached
_requestLockin the meantime. They now wait on internal observables:RequestLockContended: a call found the lock already held.RequestLockAcquired: a call has taken the lock.Both events are raised from a new
AcquireRequestLockAsynchelper and have no subscriber in production.Converted tests (the commit message lists them):
SecurityAccess_Holds_Lock_Across_Seed_And_Keynow requires a keep-alive tick to have actually queued behind SecurityAccess. Its old form counted 30 ms periods and could pass with no tick at all.DownloadAsync_Holds_Exclusive_Lock_Against_Concurrent_TesterPresentrequires both TesterPresent calls to be seen queued on the lock while the download holds it. This replaces the 2 ms race width, and the test asserts that the queuing happened.Mutation checks run locally:
An_Already_Cancelled_Call_Does_Not_Take_A_Free_Request_Lockgo red.DisposemakesN_Dispose_Leaves_The_Lock_To_A_Holder_That_Outlasts_The_Waitgo red, in 3 of 3 runs.Deliberately not in this PR: the P2/P2 time seam.* A clock seam in
UdsClientImpl(virtual arrival stamps) was prepared and then dropped. Converting three suppressed-window tests onto it removed what they check:A_Late_Negative…andSuppressed_Send_Windows_Are_Kept_Per_Servicestayed green on the converted version, and both go red onmain.A_Cancelled_Wait_Keeps_The_Rest_Of_The_Windowstayed green, in 3 of 3 runs.The reason: the window wait is bounded by a real
CancellationTokenSource. Whether the late response or the next request's transmit comes first is therefore decided on the wall clock. Once the ECU's delay becomes a virtual advance, the response arrives before that transmit and is discarded as stale, whatever the window does. I'll record the details on #171.Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no release!in the title, plus aBREAKING CHANGE:footer explaining the migration)Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds (with-p:CI=true)dotnet test CanKit.Pro.sln -c Releasepasses (net10.0, locally)FR-RAW-031,ADR-7), if any (Replace wall-clock category-2 test sleeps (macOS flake risk) #171)🤖 Generated with Claude Code
https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
Generated by Claude Code